-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #18302: Make possible to give a revision for directives in rule and get correct technique files #3247
Conversation
c8a6137
to
f39f6f6
Compare
webapp/sources/ldap-inventory/inventory-repository/src/main/resources/ldap/inventory.schema
Show resolved
Hide resolved
...ources/rudder/rudder-core/src/main/scala/com/normation/cfclerk/domain/TechniqueVersion.scala
Outdated
Show resolved
Hide resolved
...dder/rudder-core/src/main/scala/com/normation/cfclerk/services/impl/GitTechniqueReader.scala
Outdated
Show resolved
Hide resolved
.../rudder-core/src/main/scala/com/normation/rudder/configuration/ConfigurationRepository.scala
Outdated
Show resolved
Hide resolved
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/db/Doobie.scala
Outdated
Show resolved
Hide resolved
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/RudderDit.scala
Outdated
Show resolved
Hide resolved
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/RudderDit.scala
Outdated
Show resolved
Hide resolved
...udder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPDirectiveRepository.scala
Show resolved
Hide resolved
f39f6f6
to
9a2e475
Compare
9a2e475
to
6b1da3f
Compare
6b1da3f
to
ad00f1b
Compare
...ources/rudder/rudder-core/src/main/scala/com/normation/cfclerk/domain/TechniqueVersion.scala
Outdated
Show resolved
Hide resolved
...urces/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/policies/Directive.scala
Outdated
Show resolved
Hide resolved
...ources/rudder/rudder-core/src/main/scala/com/normation/cfclerk/domain/TechniqueVersion.scala
Outdated
Show resolved
Hide resolved
.../rudder-core/src/main/scala/com/normation/rudder/configuration/ConfigurationRepository.scala
Show resolved
Hide resolved
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/db/DB.scala
Show resolved
Hide resolved
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/db/Doobie.scala
Show resolved
Hide resolved
...r-core/src/main/scala/com/normation/rudder/repository/jdbc/HistorizationJdbcRepository.scala
Outdated
Show resolved
Hide resolved
webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/db/DB.scala
Outdated
Show resolved
Hide resolved
webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/JsonSchema.scala
Outdated
Show resolved
Hide resolved
webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/RuleApi.scala
Outdated
Show resolved
Hide resolved
If the toString changes and the zio-json would be in a different PR that would have been easier to read !! |
I reviewed the PR, apart some naming issues / organisation with id and revId this is ok to me Only thing is why did we have zio-json in this PR and also it would have been easier to read with the toString changes in another PR |
ad00f1b
to
17cfed2
Compare
...ources/rudder/rudder-core/src/main/scala/com/normation/cfclerk/domain/TechniqueVersion.scala
Show resolved
Hide resolved
* For backward compatibility, the UID field is named "id" in most serialized format. We will keep | ||
* "uid" in code to avoid code looking like `directive.id.id`. | ||
*/ | ||
final case class DirectiveId(uid: DirectiveUid, rev: Revision = GitVersion.defaultRev) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here with default value
...pp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/policies/Rule.scala
Outdated
Show resolved
Hide resolved
*/ | ||
def getDirective(id: DirectiveId, revId: RevId): IOResult[Option[(ActiveTechnique, Directive)]] = { | ||
def getDirective(uid: DirectiveUid, rev: Revision): IOResult[Option[(ActiveTechnique, Directive)]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a DirectiveId which regroup both ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to emphasis the fact that you look for a specific revision explictly here. I latter change the named to getDirectiveRevision
for that
@@ -197,7 +198,7 @@ class DirectiveSerialisationImpl(xmlVersion:String) extends DirectiveSerialisati | |||
, directive : Directive | |||
) = { | |||
createTrimedElem(XML_TAG_DIRECTIVE, xmlVersion) ( | |||
<id>{directive.id.value}</id> | |||
<id>{directive.id.uid.value}</id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we store the revision here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, by definition, it will be given by the git commit
...ces/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitFindUtils.scala
Outdated
Show resolved
Hide resolved
...udder/rudder-core/src/test/scala/com/normation/rudder/services/policies/NodeConfigData.scala
Outdated
Show resolved
Hide resolved
f07878e
to
0a48655
Compare
...udder/rudder-core/src/test/scala/com/normation/rudder/services/policies/NodeConfigData.scala
Outdated
Show resolved
Hide resolved
@@ -592,6 +598,22 @@ class DirectiveApiService14 ( | |||
} | |||
} | |||
|
|||
def getTechniqueWithVerion(optTechniqueName: Option[TechniqueName], opTechniqueVersion: Option[TechniqueVersion], revision: Option[Revision]): IOResult[Technique] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in function Name (Verion)
873354b
to
7b3b62f
Compare
PR rebased |
7b3b62f
to
262c7c3
Compare
This PR is not mergeable to upper versions. |
… and get correct technique files
262c7c3
to
60af039
Compare
OK, merging this PR |
https://issues.rudder.io/issues/18302
First version that allows to use past revisions of Techniques and Directives in rule (and directly from directive page by adding
+commid
near directive UUID.This is still WIP as we need to have API for directive (and unit tests) allowing to get a specific revision, and set revision for directives in rules (API to list revisions, clone a specific revision, create or clone on a branche different from master will be done in next steps, along with revision for rules).
Since first iteration of draft, main changes are:
revision
is the chosen name to talk about the commit identifier. So there is no moreRevId
,revisionId
, etc, and everything should be normalized torevision
(orrev
when abbreviated in code, but never in serialized form) ;revision
is a mandatory part of aDirectiveId
which is now defined as:The old "DirectiveId" type is renamed into
DirectiveUid
(for "unique identifier") and the correspondinguid
attribute name is used in the newDirectiveId
to refer to it.uid
was prefed touuid
because 1/ that allows to use that name if needed even for system rules/directives for some have an human readable name (system ones), and 2/ perhaps we will want to change from uuid to something else latter on.Below, the updated list of main changes:
revision
for directive and allow to specify it in rule: lots of files change, but it's because we change directive identification fromDirectiveId
toDirectiveUid + Revision
config revision
+facts revision
and lookup in git repositories all info ;TechniqueVersion
to addRevision
to it, and corresponding logic to fetch technique matching directive id (by default or revision specified in directive)See also internal documentation (development notes and updated architecture in https://www.notion.so/Versionning-des-objets-de-configuration-0d29f58ac58a42d38551a9fe118fe713)